Review requested:

• @nodejs/streams

@mcollina @ronag

i added the following tests

{
  const streamObj = new Readable();
  assert.throws(
    () => {
      // Passing autoCleanup option not as boolean
      // should throw error
      finished(streamObj, {autoCleanup: 2});
    },
    {code: 'ERR_INVALID_ARG_TYPE'}
  );
}

// Below code should not throw any errors as the
// streamObj is `Stream` and autoCleanup is boolean
{
  const streamObj = new Readable();
  finished(streamObj, {autoCleanup: true})
}

In addition, I want to make sure that the cleanup function has been called as expected

I didn't find any example of how to detect listeners has been properly removed

I see it hasn't been tested here as well :

https://github.com/nodejs/node/blob/main/test/parallel/test-stream-end-of-streams.js

Can you please help me think of a simple way to check
this cases :
cleanup function should be called when autoCleanup is set to true
cleanup function should not be called when autoCleanup is set to false
cleanup function should not be called if no value passed to autoCleanup

@mcollina @ronag

i added the following tests

{
  const streamObj = new Readable();
  assert.throws(
    () => {
      // Passing autoCleanup option not as boolean
      // should throw error
      finished(streamObj, {autoCleanup: 2});
    },
    {code: 'ERR_INVALID_ARG_TYPE'}
  );
}

// Below code should not throw any errors as the
// streamObj is `Stream` and autoCleanup is boolean
{
  const streamObj = new Readable();
  finished(streamObj, {autoCleanup: true})
}

In addition, I want to make sure that the cleanup function has been called as expected

I didn't find any example of how to detect listeners has been properly removed

I see it hasn't been tested here as well :

https://github.com/nodejs/node/blob/main/test/parallel/test-stream-end-of-streams.js

Can you please help me think of a simple way to check this cases : cleanup function should be called when autoCleanup is set to true cleanup function should not be called when autoCleanup is set to false cleanup function should not be called if no value passed to autoCleanup

@mcollina @ronag

i added the following tests

{
  const streamObj = new Readable();
  assert.throws(
    () => {
      // Passing autoCleanup option not as boolean
      // should throw error
      finished(streamObj, {autoCleanup: 2});
    },
    {code: 'ERR_INVALID_ARG_TYPE'}
  );
}

// Below code should not throw any errors as the
// streamObj is `Stream` and autoCleanup is boolean
{
  const streamObj = new Readable();
  finished(streamObj, {autoCleanup: true})
}

In addition, I want to make sure that the cleanup function has been called as expected

I didn't find any example of how to detect listeners has been properly removed

I see it hasn't been tested here as well :

https://github.com/nodejs/node/blob/main/test/parallel/test-stream-end-of-streams.js

Can you please help me think of a simple way to check this cases : cleanup function should be called when autoCleanup is set to true cleanup function should not be called when autoCleanup is set to false cleanup function should not be called if no value passed to autoCleanup

Thank you anyway, I was able to find a simple solution
just to use the listenerCount

{
  const streamObj = new Writable();
  assert(streamObj.listenerCount('end') === 0);
  finished(streamObj, {autoCleanup: false}).then(() => {
    assert(streamObj.listenerCount('end') === 1);
  })
}

CI: https://ci.nodejs.org/job/node-test-pull-request/47103/

Hey I was looking though good first issues for hacktoberfest and I saw #44556. After looking at this pull request, I wondered whether or not the cleanup function was actually being called. From what I understand, I would suspect we'd need to do something like this in order for us to call the cleanup function.

function finished(stream, opts) {
  ...
  return new Promise((resolve, reject) => {
    let result;
    const cleanup = eos(stream, opts, (err) => {
      if (err) {
        result = err;
      }
    });
    if (autoCleanup) {
      cleanup();
    }
    if (result) {
      reject(result);
    } else {
      resolve();
    }
  });
}

I made the following changes to the tests.

diff --git a/test/parallel/test-stream-promises.js b/test/parallel/test-stream-promises.js
index fae2c37492..ae98804776 100644
--- a/test/parallel/test-stream-promises.js
+++ b/test/parallel/test-stream-promises.js
@@ -120,7 +120,7 @@ assert.strictEqual(finished, promisify(stream.finished));
   const streamObj = new Writable();
   assert.strictEqual(streamObj.listenerCount('end'), 0);
   finished(streamObj, { cleanup: false }).then(() => {
-    assert.strictEqual(streamObj.listenerCount('end'), 1);
+    assert.strictEqual(streamObj.listenerCount('end'), "FOO");
   });
 }
 
@@ -130,7 +130,7 @@ assert.strictEqual(finished, promisify(stream.finished));
   const streamObj = new Writable();
   assert.strictEqual(streamObj.listenerCount('end'), 0);
   finished(streamObj, { cleanup: true }).then(() => {
-    assert.strictEqual(streamObj.listenerCount('end'), 0);
+    assert.strictEqual(streamObj.listenerCount('end'), "FOO");
   });
 }

Even with these changes, the tests still passed.

> ./tools/test.py test/parallel/test-stream-promises.js
[00:00|% 100|+   1|-   0]: Done

Are we certain the cleanup function is actually being called here? If so, would someone mind walking through how that works?

CI: https://ci.nodejs.org/job/node-test-pull-request/47269/

CI: https://ci.nodejs.org/job/node-test-pull-request/47279/

CI: https://ci.nodejs.org/job/node-test-pull-request/47280/

Landed in 84064bf